Updates to Fix Issues from Improved Forms Engine Plugin Package#167
Updates to Fix Issues from Improved Forms Engine Plugin Package#167
Conversation
|
| componentValue === undefined || | ||
| componentValue === '' | ||
| ) { | ||
| if (componentValue === undefined || componentValue === '') { |
There was a problem hiding this comment.
are we excluding null case as its handled elsewhere?
There was a problem hiding this comment.
It cannot be null.
There was a problem hiding this comment.
Good start, but it's a bit unusual to see so many casting operations /** @type {Type} /* (...) in lower levels of code. I'd usually expect a type assignment further upstream e.g. /** @type {MyType} */ const myVar = ...
Usually the compiler is pretty good at figuring out the types, so I would hope that a lot of the types aren't neccessary. If this was properly set (either via our existing FormModel type or from types set on the initial var creation), I imagine we wouldn't need much patching further downstream.
|
|
||
| const answer = field.getDisplayStringFromFormValue(mappedRichFormValue) | ||
| const answer = field.getDisplayStringFromFormValue( | ||
| /** @type {any} */ (mappedRichFormValue) |
There was a problem hiding this comment.
why do we need to cast this to any?
There was a problem hiding this comment.
Without it, we end up with an error:
Argument of type 'RichFormValue | FormAdapterFile[] | null' is not assignable to parameter of type '(((((((((((string | number | boolean | (string | number | boolean)[]) & (string | number | boolean)[]) & DatePartsState) & (string | number | boolean | (string | ... 1 more ... | boolean)[] | UploadState | RepeatListState | FeatureCollection | FormPayload)) & (string | ... 6 more ... | FormPayload)) & FileState[]) &...'.
Type 'null' is not assignable to type '(((((((((((string | number | boolean | (string | number | boolean)[]) & (string | number | boolean)[]) & DatePartsState) & (string | number | boolean | (string | ... 1 more ... | boolean)[] | UploadState | RepeatListState | FeatureCollection | FormPayload)) & (string | ... 6 more ... | FormPayload)) & FileState[]) &...'.
It doesn't match any of the types in the FormComponent.d.ts types, so the FEP would require a further update to make this without it.
There was a problem hiding this comment.
I've created a new ticket to address further issues here: https://eaflood.atlassian.net/browse/DF-1028
We should prevent any type when linting after we fix the remaining issues.
| answer, | ||
| field, | ||
| richFormValue, | ||
| /** @type {RichFormValue} */ (/** @type {unknown} */ (richFormValue)), |
There was a problem hiding this comment.
type any on L101, as unkown as RichFormValue here?
There was a problem hiding this comment.
It's because it can be any of these: RichFormValue | FormAdapterFile[] | null, and the function only accepts the first one.
| const formModel = new FormModel( | ||
| formDefinition, | ||
| { basePath: '' }, | ||
| /** @type {any} */ ({}) |
There was a problem hiding this comment.
can this type be removed or a proper type used? the previous argument doesn't have an explicit type.
There was a problem hiding this comment.
No, because it doesn't meet the contract of the required Services type and omitting it could change the behaviour because there's a default parameter value. So to properly fix it may require a change in behaviour.
| const formAdapterFiles = /** @type {FormAdapterFile[]} */ ( | ||
| /** @type {unknown} */ (richFormValue) | ||
| ) |
There was a problem hiding this comment.
better to use a type guard here rather than as unknown as FormAdapterFile[]?
There was a problem hiding this comment.
The ideal solution would be to have the function accept the correct type in the first place. But, because of the way it's used in this file, it shouldn't be necessary (ie it's called when some other piece of data indicates that this is the type). It's a question of how many changes do we want to make for this?
| if (field instanceof ListFormComponent && field instanceof FormComponent) { | ||
| return formatListFormComponent(answer, field, richFormValue) |
There was a problem hiding this comment.
what was wrong with the helper? it's used a few times
There was a problem hiding this comment.
It was just used once and better inline, unless I'm missing something?
| const formAdapterFiles = /** @type {FormAdapterFile[]} */ ( | ||
| /** @type {unknown} */ (richFormValue) | ||
| ) |
There was a problem hiding this comment.
another that would benefit from a type guard
| } | ||
|
|
||
| const itemLabel = `${repeaterTitle} ${i + 1}` | ||
| const formField = /** @type {FormComponent} */ (componentField) |
There was a problem hiding this comment.
can the type be set upstream ather than casting it at this lower level?
There was a problem hiding this comment.
Possibly, but it would require other changes which could expand.
3 participants
An update to Forms Engine Plugin means that the types will now correctly resolve using relative paths for the type declarations, instead of using the tilde (~) which did not resolve within the consuming project. This fixes new linting issues that result.